Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sys/shell: print error code of command #17922

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

sys/shell: print error code of command #17922

wants to merge 1 commit into from

Conversation

benpicco
Copy link
Contributor

Contribution description

I've written code like

int _command_handler(int argc, char **argv)
{
    int res = do_something();
    if (res) {
        printf("error: %d\n");
    }
    return res;
}

way too often.
Let's let the shell always print the error code of shell commands, as it is otherwise just discarded.

Testing procedure

Try your favorite shell commands.

Issues/PRs references

@benpicco benpicco requested a review from kaspar030 April 12, 2022 20:49
@github-actions github-actions bot added the Area: sys Area: System label Apr 12, 2022
@benpicco benpicco requested review from maribu and MrKevinWeiss April 12, 2022 20:49
@benpicco benpicco changed the title sys/shell: print error code of commands sys/shell: print error code of command Apr 12, 2022
@benpicco benpicco added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Apr 12, 2022
@maribu
Copy link
Member

maribu commented Apr 12, 2022

What about exit codes > 0? I think e.g. EXIT_SUCCESS is mostly 0, but EXIT_FAILURE often is postive. Maybe print either all exit codes, or all except EXIT_SUCCEES?

This might break some shell abstractions btw. that habe baked in some expectations about the shell output format.

@benpicco benpicco added the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Apr 12, 2022
@benpicco
Copy link
Contributor Author

I just noticed that some commands would print positive error if the number of arguments was wrong and they would print a help instead.
Printing the error there would be quite noisy.

@benpicco benpicco mentioned this pull request Apr 15, 2022
Merged
@benpicco benpicco requested a review from chrysn April 15, 2022 13:30
@chrysn
Copy link
Member

chrysn commented Apr 15, 2022

For the "print help" the error code may not be as helpful, as the error message ("Usage: %s gadget [--arg]") is not what the command line parser knows anyway.

Suggestion: On negative codes, print strerror(e); on positive, keep relying on the command to have printed its error. (The codes might still later get more effects, like setting $?).

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 17, 2022
benpicco
benpicco commented Apr 22, 2022
View reviewed changes
Copy link
Contributor Author

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm now strerror() adds too much ROM on small boards (and kills the AVR family where this is then placed in RAM).

This wants me to have multiple levels of DEVELHELP again so we could set DEVELHELP=2 to get these more expensive features.

@benpicco benpicco mentioned this pull request May 9, 2022
Merged
@stale
Copy link

stale bot commented Nov 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@maribu maribu Awaiting requested review from maribu

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

@chrysn chrysn Awaiting requested review from chrysn

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer State: stale State: The issue / PR has no activity for >185 days
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benpicco @maribu @chrysn